Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: populate context from config #452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

BPR02
Copy link

@BPR02 BPR02 commented Nov 25, 2024

- populate ctx.data and ctx.assets with the following info from the config:
  - name
  - description
  - pack_format
  - supported_formats
@BPR02
Copy link
Author

BPR02 commented Jan 14, 2025

@misode @RitikShah @edayot I think this PR can be merged. I tested it with my Observer plugin and it seems to be working as expected. I'd like a review if you guys have time.

I added some tests, but there might be a more elegant way to test the output... I'm not super familiar with pytest, but I couldn't find anything online about passing an output to an object for pytest to catch, so I just put the text into a meta field and checked it afterwards.

@@ -274,6 +274,26 @@ def build(self) -> Iterator[Context]:
whitelist=self.config.whitelist,
)

# populate ctx.pack with config info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theses changes are good

@pytest.mark.parametrize("directory", os.listdir("tests/ctx_config_examples"))
def test_examples(snapshot: SnapshotFixture, directory: str):
with run_beet(directory=f"tests/ctx_config_examples/{directory}") as ctx:
assert snapshot() == f'{ctx.meta["pytest"]}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here instead of creating a snapshot from f'{ctx.meta["pytest"]}', it's maybe better to it like this :

with run_beet(directory=f"tests/ctx_config_examples/{directory}") as ctx:
    assert snapshot() == {
        "minecraft_version": ctx.minecraft_version,
        "project_author": ctx.project_author,
        # (... with all other parts of the config transferred to the ctx variable )
        }

This would eliminate the need of per directory plugin, and the need of a the new subfolder ctx_config_examples in favor of config_examples (transferring all examples to the old folder).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug (#451) only occurs when trying to read the values in a plugin, so checking it in the test function will give a different result. Basically, the problem is that plugins can't access the info because it's populated only when building the pack (i.e. after running the plugins). Since run_beet populates ctx as if it was built to completion, the bug isn't caught by checking ctx in the test function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh i see, i don't like the ctx.meta["pytest"] access, but it's the only way i can think of accessing plugin phase value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was the main reason I wanted a review; the ctx.meta["pytest"] feels really janky, but I couldn't find another way to check those values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ctx.data and ctx.assets are not populated by the config
2 participants